[clang] Fix PointerAuth semantics of cpp_trivially_relocatable #143796
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…atable This reworks the way we compute relocatability and replaceability of types. We do this by having a single interface the provides a full `TypeRelocationInfo` for a QualType. This simplifies the reasoning for all cases, and reduces duplication of code. We still limit our caching of results to CXXRecordDecls but i think it might be worth caching on canonical types in future, but for now I didn't want to change the caching semantics any further. TypeRelocationInfo no longer permits raw field access as we can then ensure we maintain correct state over pointer auth and similar. As a basic optimization we gate the additional required work on the existence pointer auth options.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Oliver Hunt (ojhunt) ChangesThis reworks the way we compute relocatability and replaceability of types. We do this by having a single interface the provides a full We still limit our caching of results to CXXRecordDecls but i think it might be worth caching on canonical types in future, but for now I didn't want to change the caching semantics any further. TypeRelocationInfo no longer permits raw field access as we can then ensure we maintain correct state over pointer auth and similar. As a basic optimization we gate the additional required work on the existence pointer auth options. Patch is 23.04 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/143796.diff 8 Files Affected:
diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h
index 8d24d393eab09..4526189a83348 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -620,18 +620,61 @@ class ASTContext : public RefCountedBase<ASTContext> {
ParameterIndexTable ParamIndices;
public:
- struct CXXRecordDeclRelocationInfo {
- unsigned IsRelocatable;
- unsigned IsReplaceable;
+ struct TypeRelocationInfo {
+ static TypeRelocationInfo invalid() {
+ TypeRelocationInfo Result;
+ Result.IsRelocatable = false;
+ Result.IsReplaceable = false;
+ return Result;
+ }
+
+ TypeRelocationInfo() {}
+ TypeRelocationInfo(bool IsRelocatable, bool IsReplaceable,
+ bool HasAddressDiscriminatedValues)
+ : IsUnion(false), IsRelocatable(IsRelocatable),
+ IsReplaceable(IsReplaceable),
+ HasAddressDiscriminatedValues(HasAddressDiscriminatedValues) {}
+
+ void setIsUnion(bool Union) {
+ IsUnion = Union;
+ normalizeState();
+ }
+ void updateRelocatable(bool Relocatable) { IsRelocatable &= Relocatable; }
+ void updateReplaceable(bool Replaceable) { IsReplaceable &= Replaceable; }
+ void
+ updateContainsAddressDiscriminatedValues(bool AddressDiscriminatedValues) {
+ HasAddressDiscriminatedValues |= AddressDiscriminatedValues;
+ normalizeState();
+ }
+ void mergeWithSiblingMember(bool IsUnion, const TypeRelocationInfo &Other) {
+ updateRelocatable(Other.IsRelocatable);
+ updateReplaceable(Other.IsReplaceable);
+ normalizeState();
+ }
+ bool isRelocatable() const { return IsRelocatable; }
+ bool isReplaceable() const { return IsReplaceable; }
+ bool hasAddressDiscriminatedPointerAuth() const {
+ return HasAddressDiscriminatedValues;
+ }
+
+ private:
+ void normalizeState() {
+ if (!IsUnion || !HasAddressDiscriminatedValues)
+ return;
+ IsRelocatable = false;
+ IsReplaceable = false;
+ }
+ bool IsUnion = false;
+ bool IsRelocatable = true;
+ bool IsReplaceable = true;
+ bool HasAddressDiscriminatedValues = false;
};
- std::optional<CXXRecordDeclRelocationInfo>
+ std::optional<TypeRelocationInfo>
getRelocationInfoForCXXRecord(const CXXRecordDecl *) const;
- void setRelocationInfoForCXXRecord(const CXXRecordDecl *,
- CXXRecordDeclRelocationInfo);
+ void setRelocationInfoForCXXRecord(const CXXRecordDecl *, TypeRelocationInfo);
private:
- llvm::DenseMap<const CXXRecordDecl *, CXXRecordDeclRelocationInfo>
- RelocatableClasses;
+ llvm::DenseMap<const CXXRecordDecl *, TypeRelocationInfo> RelocationInfo;
ImportDecl *FirstLocalImport = nullptr;
ImportDecl *LastLocalImport = nullptr;
@@ -3668,6 +3711,7 @@ OPT_LIST(V)
/// authentication policy for the specified record.
const CXXRecordDecl *
baseForVTableAuthentication(const CXXRecordDecl *ThisClass);
+ bool hasAddressDiscriminatedVTableAuthentication(const CXXRecordDecl *Class);
bool useAbbreviatedThunkName(GlobalDecl VirtualMethodDecl,
StringRef MangledName);
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 0dad07e55a820..5e405c604598a 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -4343,8 +4343,10 @@ class Sema final : public SemaBase {
void ActOnTagFinishDefinition(Scope *S, Decl *TagDecl,
SourceRange BraceRange);
- ASTContext::CXXRecordDeclRelocationInfo
- CheckCXX2CRelocatableAndReplaceable(const clang::CXXRecordDecl *D);
+ ASTContext::TypeRelocationInfo
+ GetCXX2CTypeRelocationInfo(const clang::CXXRecordDecl *D);
+
+ ASTContext::TypeRelocationInfo GetCXX2CTypeRelocationInfo(QualType T);
void ActOnTagFinishSkippedDefinition(SkippedDefinitionContext Context);
@@ -8680,7 +8682,6 @@ class Sema final : public SemaBase {
// FIXME: This is in Sema because it requires
// overload resolution, can we move to ASTContext?
bool IsCXXTriviallyRelocatableType(QualType T);
- bool IsCXXTriviallyRelocatableType(const CXXRecordDecl &RD);
//// Determines if a type is replaceable
/// according to the C++26 rules.
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index b51f7622288df..b4f1133e6adcd 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -1687,22 +1687,22 @@ void ASTContext::getOverriddenMethods(
Overridden.append(OverDecls.begin(), OverDecls.end());
}
-std::optional<ASTContext::CXXRecordDeclRelocationInfo>
+std::optional<ASTContext::TypeRelocationInfo>
ASTContext::getRelocationInfoForCXXRecord(const CXXRecordDecl *RD) const {
assert(RD);
CXXRecordDecl *D = RD->getDefinition();
- auto it = RelocatableClasses.find(D);
- if (it != RelocatableClasses.end())
+ auto it = RelocationInfo.find(D);
+ if (it != RelocationInfo.end())
return it->getSecond();
return std::nullopt;
}
-void ASTContext::setRelocationInfoForCXXRecord(
- const CXXRecordDecl *RD, CXXRecordDeclRelocationInfo Info) {
+void ASTContext::setRelocationInfoForCXXRecord(const CXXRecordDecl *RD,
+ TypeRelocationInfo Info) {
assert(RD);
CXXRecordDecl *D = RD->getDefinition();
- assert(RelocatableClasses.find(D) == RelocatableClasses.end());
- RelocatableClasses.insert({D, Info});
+ assert(RelocationInfo.find(D) == RelocationInfo.end());
+ RelocationInfo.insert({D, Info});
}
void ASTContext::addedLocalImportDecl(ImportDecl *Import) {
@@ -15121,6 +15121,21 @@ ASTContext::baseForVTableAuthentication(const CXXRecordDecl *ThisClass) {
return PrimaryBase;
}
+bool ASTContext::hasAddressDiscriminatedVTableAuthentication(
+ const CXXRecordDecl *Class) {
+ assert(Class->isPolymorphic());
+ const CXXRecordDecl *BaseType = baseForVTableAuthentication(Class);
+ using AuthAttr = VTablePointerAuthenticationAttr;
+ const auto *ExplicitAuth = BaseType->getAttr<AuthAttr>();
+ if (!ExplicitAuth)
+ return LangOpts.PointerAuthVTPtrAddressDiscrimination;
+ AuthAttr::AddressDiscriminationMode AddressDiscrimination =
+ ExplicitAuth->getAddressDiscrimination();
+ if (AddressDiscrimination == AuthAttr::DefaultAddressDiscrimination)
+ return LangOpts.PointerAuthVTPtrAddressDiscrimination;
+ return AddressDiscrimination == AuthAttr::AddressDiscrimination;
+}
+
bool ASTContext::useAbbreviatedThunkName(GlobalDecl VirtualMethodDecl,
StringRef MangledName) {
auto *Method = cast<CXXMethodDecl>(VirtualMethodDecl.getDecl());
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index 39d4d49a0fe79..8e79e975dc270 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -10615,7 +10615,7 @@ void Sema::checkIllFormedTrivialABIStruct(CXXRecordDecl &RD) {
}
}
- if (IsCXXTriviallyRelocatableType(RD))
+ if (GetCXX2CTypeRelocationInfo(&RD).isRelocatable())
return;
// Ill-formed if the copy and move constructors are deleted.
diff --git a/clang/lib/Sema/SemaTypeTraits.cpp b/clang/lib/Sema/SemaTypeTraits.cpp
index 1738ab4466001..e2f1322df42a1 100644
--- a/clang/lib/Sema/SemaTypeTraits.cpp
+++ b/clang/lib/Sema/SemaTypeTraits.cpp
@@ -226,12 +226,13 @@ static bool IsEligibleForReplacement(Sema &SemaRef, const CXXRecordDecl *D) {
return !D->hasDeletedDestructor();
}
-ASTContext::CXXRecordDeclRelocationInfo
-Sema::CheckCXX2CRelocatableAndReplaceable(const CXXRecordDecl *D) {
- ASTContext::CXXRecordDeclRelocationInfo Info{false, false};
+ASTContext::TypeRelocationInfo
+Sema::GetCXX2CTypeRelocationInfo(const CXXRecordDecl *D) {
+ if (auto ExistingInfo = Context.getRelocationInfoForCXXRecord(D))
+ return *ExistingInfo;
if (!getLangOpts().CPlusPlus || D->isInvalidDecl())
- return Info;
+ return ASTContext::TypeRelocationInfo::invalid();
assert(D->hasDefinition());
@@ -244,12 +245,11 @@ Sema::CheckCXX2CRelocatableAndReplaceable(const CXXRecordDecl *D) {
*this, D, /*AllowUserDefined=*/true);
};
- auto IsUnion = [&, Is = std::optional<bool>{}]() mutable {
+ auto IsTrivialUnion = [&, Is = std::optional<bool>{}]() mutable {
if (!Is.has_value())
Is = D->isUnion() && !D->hasUserDeclaredCopyConstructor() &&
!D->hasUserDeclaredCopyAssignment() &&
- !D->hasUserDeclaredMoveOperation() &&
- !D->hasUserDeclaredDestructor();
+ !D->hasUserDeclaredMoveOperation() && !D->hasUserDeclaredDestructor();
return *Is;
};
@@ -259,7 +259,9 @@ Sema::CheckCXX2CRelocatableAndReplaceable(const CXXRecordDecl *D) {
return *Is;
};
- Info.IsRelocatable = [&] {
+ ASTContext::TypeRelocationInfo Info;
+
+ Info.updateRelocatable([&] {
if (D->isDependentType())
return false;
@@ -272,14 +274,14 @@ Sema::CheckCXX2CRelocatableAndReplaceable(const CXXRecordDecl *D) {
return true;
// is a union with no user-declared special member functions, or
- if (IsUnion())
+ if (IsTrivialUnion())
return true;
// is default-movable.
return IsDefaultMovable();
- }();
+ }());
- Info.IsReplaceable = [&] {
+ Info.updateReplaceable([&] {
if (D->isDependentType())
return false;
@@ -292,77 +294,133 @@ Sema::CheckCXX2CRelocatableAndReplaceable(const CXXRecordDecl *D) {
return HasSuitableSMP();
// is a union with no user-declared special member functions, or
- if (IsUnion())
+ if (IsTrivialUnion())
return HasSuitableSMP();
// is default-movable.
return IsDefaultMovable();
- }();
-
+ }());
+
+ Info.setIsUnion(D->isUnion());
+
+ bool PtrauthMatters = LangOpts.PointerAuthIntrinsics ||
+ LangOpts.PointerAuthVTPtrAddressDiscrimination;
+ if (PtrauthMatters) {
+ auto IsBottomRelocationInfo =
+ [](const ASTContext::TypeRelocationInfo &Info) {
+ return !Info.isReplaceable() && !Info.isRelocatable() &&
+ Info.hasAddressDiscriminatedPointerAuth();
+ };
+
+ if (D->isPolymorphic())
+ Info.updateContainsAddressDiscriminatedValues(
+ Context.hasAddressDiscriminatedVTableAuthentication(D));
+ for (auto Base : D->bases()) {
+ if (IsBottomRelocationInfo(Info))
+ break;
+ bool BaseHasPtrauth = GetCXX2CTypeRelocationInfo(Base.getType())
+ .hasAddressDiscriminatedPointerAuth();
+ Info.updateContainsAddressDiscriminatedValues(BaseHasPtrauth);
+ }
+ for (auto *FieldDecl : D->fields()) {
+ if (IsBottomRelocationInfo(Info))
+ break;
+ bool FieldHasPtrauth = GetCXX2CTypeRelocationInfo(FieldDecl->getType())
+ .hasAddressDiscriminatedPointerAuth();
+ Info.updateContainsAddressDiscriminatedValues(FieldHasPtrauth);
+ }
+ }
+ Context.setRelocationInfoForCXXRecord(D, Info);
return Info;
}
-bool Sema::IsCXXTriviallyRelocatableType(const CXXRecordDecl &RD) {
- if (std::optional<ASTContext::CXXRecordDeclRelocationInfo> Info =
- getASTContext().getRelocationInfoForCXXRecord(&RD))
- return Info->IsRelocatable;
- ASTContext::CXXRecordDeclRelocationInfo Info =
- CheckCXX2CRelocatableAndReplaceable(&RD);
- getASTContext().setRelocationInfoForCXXRecord(&RD, Info);
- return Info.IsRelocatable;
-}
+ASTContext::TypeRelocationInfo Sema::GetCXX2CTypeRelocationInfo(QualType T) {
+ T = T.getCanonicalType();
+ enum class DirectRelocationInformation { Yes, No, Unknown };
+ DirectRelocationInformation Relocatable =
+ DirectRelocationInformation::Unknown;
+ DirectRelocationInformation Replaceable =
+ DirectRelocationInformation::Unknown;
+ DirectRelocationInformation ContainsAddressDiscriminatedValues =
+ DirectRelocationInformation::Unknown;
+
+ auto UpdateRelocatable = [&](DirectRelocationInformation DRI) {
+ if (Relocatable == DirectRelocationInformation::Unknown ||
+ Relocatable == DirectRelocationInformation::Yes)
+ Relocatable = DRI;
+ };
+ auto UpdateReplaceable = [&](DirectRelocationInformation DRI) {
+ if (Replaceable == DirectRelocationInformation::Unknown ||
+ Replaceable == DirectRelocationInformation::Yes)
+ Replaceable = DRI;
+ };
+ auto UpdateAddressDiscrimination = [&](DirectRelocationInformation DRI) {
+ if (ContainsAddressDiscriminatedValues ==
+ DirectRelocationInformation::Unknown ||
+ ContainsAddressDiscriminatedValues == DirectRelocationInformation::No)
+ ContainsAddressDiscriminatedValues = DRI;
+ };
-bool Sema::IsCXXTriviallyRelocatableType(QualType Type) {
+ if (T->isVariableArrayType()) {
+ UpdateRelocatable(DirectRelocationInformation::No);
+ UpdateReplaceable(DirectRelocationInformation::No);
+ }
- QualType BaseElementType = getASTContext().getBaseElementType(Type);
+ if (T.isConstQualified() || T.isVolatileQualified())
+ UpdateReplaceable(DirectRelocationInformation::No);
- if (Type->isVariableArrayType())
- return false;
+ if (T.hasAddressDiscriminatedPointerAuth())
+ UpdateAddressDiscrimination(DirectRelocationInformation::Yes);
+
+ QualType BaseElementType =
+ SemaRef.getASTContext().getBaseElementType(T.getUnqualifiedType());
+
+ if (BaseElementType->isIncompleteType()) {
+ Relocatable = DirectRelocationInformation::No;
+ Replaceable = DirectRelocationInformation::No;
+ }
if (BaseElementType.hasNonTrivialObjCLifetime())
- return false;
+ UpdateRelocatable(DirectRelocationInformation::No);
- if (BaseElementType.hasAddressDiscriminatedPointerAuth())
- return false;
+ if (BaseElementType->isScalarType()) {
+ UpdateRelocatable(DirectRelocationInformation::Yes);
+ UpdateReplaceable(DirectRelocationInformation::Yes);
+ UpdateAddressDiscrimination(DirectRelocationInformation::No);
+ }
- if (BaseElementType->isIncompleteType())
- return false;
+ if (BaseElementType->isVectorType())
+ UpdateRelocatable(DirectRelocationInformation::Yes);
- if (BaseElementType->isScalarType() || BaseElementType->isVectorType())
- return true;
+ auto CreateInfo = [=]() -> ASTContext::TypeRelocationInfo {
+ ASTContext::TypeRelocationInfo Result;
+ return {Relocatable == DirectRelocationInformation::Yes,
+ Replaceable == DirectRelocationInformation::Yes,
+ ContainsAddressDiscriminatedValues ==
+ DirectRelocationInformation::Yes};
+ };
- if (const auto *RD = BaseElementType->getAsCXXRecordDecl())
- return IsCXXTriviallyRelocatableType(*RD);
+ if (BaseElementType->isIncompleteType())
+ return CreateInfo();
- return false;
+ const CXXRecordDecl *RD = BaseElementType->getAsCXXRecordDecl();
+ if (!RD)
+ return CreateInfo();
+
+ ASTContext::TypeRelocationInfo Info = GetCXX2CTypeRelocationInfo(RD);
+ Info.updateRelocatable(Relocatable != DirectRelocationInformation::No);
+ Info.updateReplaceable(Replaceable != DirectRelocationInformation::No);
+ Info.updateContainsAddressDiscriminatedValues(
+ ContainsAddressDiscriminatedValues == DirectRelocationInformation::Yes);
+ return Info;
}
-static bool IsCXXReplaceableType(Sema &S, const CXXRecordDecl *RD) {
- if (std::optional<ASTContext::CXXRecordDeclRelocationInfo> Info =
- S.getASTContext().getRelocationInfoForCXXRecord(RD))
- return Info->IsReplaceable;
- ASTContext::CXXRecordDeclRelocationInfo Info =
- S.CheckCXX2CRelocatableAndReplaceable(RD);
- S.getASTContext().setRelocationInfoForCXXRecord(RD, Info);
- return Info.IsReplaceable;
+bool Sema::IsCXXTriviallyRelocatableType(QualType Type) {
+ return GetCXX2CTypeRelocationInfo(Type).isRelocatable();
}
bool Sema::IsCXXReplaceableType(QualType Type) {
- if (Type.isConstQualified() || Type.isVolatileQualified())
- return false;
-
- if (Type->isVariableArrayType())
- return false;
-
- QualType BaseElementType =
- getASTContext().getBaseElementType(Type.getUnqualifiedType());
- if (BaseElementType->isIncompleteType())
- return false;
- if (BaseElementType->isScalarType())
- return true;
- if (const auto *RD = BaseElementType->getAsCXXRecordDecl())
- return ::IsCXXReplaceableType(*this, RD);
- return false;
+ return GetCXX2CTypeRelocationInfo(Type).isReplaceable();
}
/// Checks that type T is not a VLA.
@@ -674,8 +732,14 @@ static bool IsTriviallyRelocatableType(Sema &SemaRef, QualType T) {
return false;
if (const auto *RD = BaseElementType->getAsCXXRecordDecl();
- RD && !RD->isPolymorphic() && SemaRef.IsCXXTriviallyRelocatableType(*RD))
- return true;
+ RD && !RD->isPolymorphic()) {
+ ASTContext::TypeRelocationInfo Info =
+ SemaRef.GetCXX2CTypeRelocationInfo(RD);
+ if (Info.hasAddressDiscriminatedPointerAuth())
+ return false;
+ if (Info.isRelocatable())
+ return true;
+ }
if (const auto *RD = BaseElementType->getAsRecordDecl())
return RD->canPassInRegisters();
diff --git a/clang/test/SemaCXX/cxx2c-trivially-relocatable.cpp b/clang/test/SemaCXX/cxx2c-trivially-relocatable.cpp
index 9d43994ee7661..7152a5937d9b7 100644
--- a/clang/test/SemaCXX/cxx2c-trivially-relocatable.cpp
+++ b/clang/test/SemaCXX/cxx2c-trivially-relocatable.cpp
@@ -1,4 +1,5 @@
// RUN: %clang_cc1 -std=c++2c -verify %s
+// RUN: %clang_cc1 -triple aarch64-linux-gnu -fptrauth-intrinsics -fptrauth-calls -std=c++2c -verify %s
class Trivial {};
static_assert(__builtin_is_cpp_trivially_relocatable(Trivial));
diff --git a/clang/test/SemaCXX/ptrauth-triviality.cpp b/clang/test/SemaCXX/ptrauth-triviality.cpp
index 60d1b57230f18..6f3650f7ac2e3 100644
--- a/clang/test/SemaCXX/ptrauth-triviality.cpp
+++ b/clang/test/SemaCXX/ptrauth-triviality.cpp
@@ -26,7 +26,7 @@ static_assert(!__is_trivially_assignable(S1, const S1&));
static_assert(__is_trivially_destructible(S1));
static_assert(!__is_trivially_copyable(S1));
static_assert(!__is_trivially_relocatable(S1)); // expected-warning{{deprecated}}
-static_assert(!__builtin_is_cpp_trivially_relocatable(S1));
+static_assert(__builtin_is_cpp_trivially_relocatable(S1));
static_assert(!__is_trivially_equality_comparable(S1));
static_assert(__is_trivially_constructible(Holder<S1>));
@@ -35,7 +35,7 @@ static_assert(!__is_trivially_assignable(Holder<S1>, const Holder<S1>&));
static_assert(__is_trivially_destructible(Holder<S1>));
static_assert(!__is_trivially_copyable(Holder<S1>));
static_assert(!__is_trivially_relocatable(Holder<S1>)); // expected-warning{{deprecated}}
-static_assert(!__builtin_is_cpp_trivially_relocatable(Holder<S1>));
+static_assert(__builtin_is_cpp_trivially_relocatable(Holder<S1>));
static_assert(!__is_trivially_equality_comparable(Holder<S1>));
struct S2 {
@@ -83,7 +83,7 @@ static_assert(!__is_trivially_constructible(Holder<S3>, const Holder<S3>&));
static_assert(!__is_trivially_assignable(Holder<S3>, const Holder<S3>&));
static_assert(__is_trivially_destructible(Holder<S3>));
static_assert(!__is_trivially_copyable(Holder<S3>));
-static_assert(__is_trivially_relocatable(Holder<S3>)); // expected-warning{{deprecated}}
+static_assert(!__is_trivially_relocatable(Holder<S3>)); // expected-warning{{deprecated}}
static_assert(__builtin_is_cpp_trivially_relocatable(Holder<S3>));
static_assert(!__is_trivially_equality_comparable(Holder<S3>));
@@ -148,7 +148,7 @@ static_assert(!__is_trivially_assignable(S6, const S6&));
static_assert(__is_trivially_destructible(S6));
static_assert(!__is_trivially_copyable(S6));
static_assert(!__is_trivially_relocatable(S6)); // expected-warning{{deprecated}}
-static_assert(!__builtin_is_cpp_trivially_relocatable(S6));
+static_assert(__builtin_is_cpp_trivially_relocatable(S6));
static_assert(!__is_trivially_equality_comparable(S6));
static_assert(__is_trivially_constructible(Holder<S6>));
@@ -157,7 +157,7 @@ static_assert(!__is_trivially_assignable(Holder<S6>, const Holder<S6>&));
static_assert(__is_trivially_destructible(Holder<S6>));
static_assert(!__is_trivially_copyable(Holder<S6>));
static_assert(!__is_trivially_relocatable(Holder<S6>)); // expected-warning{{deprecated}}
-static_assert(!__builtin_is_cpp_trivially_relocatable(Holder<S6>));
+static_assert(__builtin_is_cpp_trivially_relocatable(Holder<S6>));
static_assert(!__is_trivially_equality_comparable(Holder<S6>));
struct S7 {
diff --git a/clang/test/SemaCXX/trivi...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Codegen needs to be able to test this, so we need to be able to rely on queries to the ASTContext.
cor3ntin
reviewed
Jun 12, 2025
Comment on lines
+1726
to
+1738
| if (const CXXRecordDecl *CXXRD = dyn_cast<CXXRecordDecl>(RD)) { | ||
| if (CXXRD->isPolymorphic() && | ||
| hasAddressDiscriminatedVTableAuthentication(CXXRD)) | ||
| return SaveReturn(true); | ||
| for (auto Base : CXXRD->bases()) { | ||
| if (containsAddressDiscriminatedPointerAuth(Base.getType())) | ||
| return SaveReturn(true); | ||
| } | ||
| } | ||
| for (auto *FieldDecl : RD->fields()) { | ||
| if (containsAddressDiscriminatedPointerAuth(FieldDecl->getType())) | ||
| return SaveReturn(true); | ||
| } |
There was a problem hiding this comment.
Do we care about the C case at all? (ie, the case where a RecordDecl is not a CXXRecord decl?)
C support so far is not something I considered
There was a problem hiding this comment.
Establishing relocatability needs to do lookups, so it's expensive - this is why we only compute it on demand.
However, for "containsAddressDiscriminatedPointerAuth", maybe that's something we can establish as we build the class, and have the result stored in DefinitionData. Is that something you considered?
There was a problem hiding this comment.
I think it's reasonable to cache the result in a map, and optimize later.
Maybe we can make it work for C++ now, and open an issue for C ?
| } | ||
|
|
||
| if (IsCXXTriviallyRelocatableType(RD)) | ||
| if (CheckCXX2CRelocatableAndReplaceable(&RD).IsRelocatable) |
There was a problem hiding this comment.
I think we should keep that interface even if we deleguate to something under the hood
There was a problem hiding this comment.
I think I did restore it for QualType and was too lazy to add the RD wrapper as well
| Info.IsReplaceable = false; | ||
| } | ||
| }; | ||
| auto IsBottomRelocationInfo = [](const CXXRecordDeclRelocationInfo &Info) { |
There was a problem hiding this comment.
This name does not really speak to me
There was a problem hiding this comment.
Yeah, I really wasn't sure what to call it
Comment on lines
628
to
629
| unsigned IsRelocatable; | ||
| unsigned IsReplaceable; |
There was a problem hiding this comment.
Why are these not bool? bad, bad @cor3ntin
There was a problem hiding this comment.
I did wonder, but I assumed you had Reasons(tm)
cor3ntin
reviewed
Jun 12, 2025
Comment on lines
+340
to
+411
| Sema::CheckCXX2CRelocatableAndReplaceable(QualType T) { | ||
| T = T.getCanonicalType(); | ||
| enum class DirectRelocationInformation { Yes, No, Unknown }; | ||
| DirectRelocationInformation Relocatable = | ||
| DirectRelocationInformation::Unknown; | ||
| DirectRelocationInformation Replaceable = | ||
| DirectRelocationInformation::Unknown; | ||
| DirectRelocationInformation ContainsAddressDiscriminatedValues = | ||
| DirectRelocationInformation::Unknown; | ||
|
|
||
| auto UpdateRelocatable = [&](DirectRelocationInformation DRI) { | ||
| if (Relocatable == DirectRelocationInformation::Unknown || | ||
| Relocatable == DirectRelocationInformation::Yes) | ||
| Relocatable = DRI; | ||
| }; | ||
| auto UpdateReplaceable = [&](DirectRelocationInformation DRI) { | ||
| if (Replaceable == DirectRelocationInformation::Unknown || | ||
| Replaceable == DirectRelocationInformation::Yes) | ||
| Replaceable = DRI; | ||
| }; | ||
| auto UpdateAddressDiscrimination = [&](DirectRelocationInformation DRI) { | ||
| if (ContainsAddressDiscriminatedValues == DirectRelocationInformation::Yes) | ||
| Replaceable = DirectRelocationInformation::No; | ||
| }; | ||
|
|
||
| bool Sema::IsCXXTriviallyRelocatableType(QualType Type) { | ||
| if (T->isVariableArrayType()) { | ||
| UpdateRelocatable(DirectRelocationInformation::No); | ||
| UpdateReplaceable(DirectRelocationInformation::No); | ||
| } | ||
|
|
||
| QualType BaseElementType = getASTContext().getBaseElementType(Type); | ||
| if (T.isConstQualified() || T.isVolatileQualified()) | ||
| UpdateReplaceable(DirectRelocationInformation::No); | ||
|
|
||
| if (Type->isVariableArrayType()) | ||
| return false; | ||
| if (T.hasAddressDiscriminatedPointerAuth()) | ||
| UpdateAddressDiscrimination(DirectRelocationInformation::Yes); | ||
|
|
||
| QualType BaseElementType = | ||
| SemaRef.getASTContext().getBaseElementType(T.getUnqualifiedType()); | ||
|
|
||
| if (BaseElementType->isIncompleteType()) { | ||
| Relocatable = DirectRelocationInformation::No; | ||
| Replaceable = DirectRelocationInformation::No; | ||
| } | ||
|
|
||
| if (BaseElementType.hasNonTrivialObjCLifetime()) | ||
| return false; | ||
| UpdateRelocatable(DirectRelocationInformation::No); | ||
|
|
||
| if (BaseElementType.hasAddressDiscriminatedPointerAuth()) | ||
| return false; | ||
| if (BaseElementType->isScalarType()) { | ||
| UpdateRelocatable(DirectRelocationInformation::Yes); | ||
| UpdateReplaceable(DirectRelocationInformation::Yes); | ||
| UpdateAddressDiscrimination(DirectRelocationInformation::No); | ||
| } | ||
|
|
||
| if (BaseElementType->isIncompleteType()) | ||
| return false; | ||
| if (BaseElementType->isVectorType()) | ||
| UpdateRelocatable(DirectRelocationInformation::Yes); | ||
|
|
||
| if (BaseElementType->isScalarType() || BaseElementType->isVectorType()) | ||
| return true; | ||
| auto CreateInfo = [=]() -> CXXRecordDeclRelocationInfo { | ||
| return {Relocatable == DirectRelocationInformation::Yes, | ||
| Replaceable == DirectRelocationInformation::Yes}; | ||
| }; | ||
|
|
||
| if (const auto *RD = BaseElementType->getAsCXXRecordDecl()) | ||
| return IsCXXTriviallyRelocatableType(*RD); | ||
| if (BaseElementType->isIncompleteType()) | ||
| return CreateInfo(); | ||
|
|
||
| return false; | ||
| const CXXRecordDecl *RD = BaseElementType->getAsCXXRecordDecl(); | ||
| if (!RD) | ||
| return CreateInfo(); | ||
|
|
||
| CXXRecordDeclRelocationInfo Info = CheckCXX2CRelocatableAndReplaceable(RD); | ||
| Info.IsRelocatable &= Relocatable != DirectRelocationInformation::No; | ||
| Info.IsReplaceable &= Replaceable != DirectRelocationInformation::No; | ||
| return Info; |
There was a problem hiding this comment.
Github ate my comment earlier.
I'm not convinced that trying to merge replaceable and relocatable improves readability.
Especially given we compute them lazily, and I suspect needing to know if a type is replaceable will be fairly less frequent. I think having to (mentally) track these 3 tribools is not easy and we don;t really know how these properties will diverge over time.
So I would prefer they remain separate for the time being.
There was a problem hiding this comment.
I think I agree - when the merging happened I though the same logic also needed to track pointer auth characteristics, but later on I separated that into its own separate query via the context, which calls the earlier merge into question.
As noted on discord I may just revert Sema.h and SemaTypeTraits.cpp and just add the required union checks now that the context checks are an option
|
Closing this PR because there's a lot of noise in it that is just completely reverted/gone at this point. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This reworks the way we compute relocatability and replaceability of types. We do this by having a single interface the provides a full
TypeRelocationInfofor a QualType. This simplifies the reasoning for all cases, and reduces duplication of code.We still limit our caching of results to CXXRecordDecls but i think it might be worth caching on canonical types in future, but for now I didn't want to change the caching semantics any further.
TypeRelocationInfo no longer permits raw field access as we can then ensure we maintain correct state over pointer auth and similar.
As a basic optimization we gate the additional required work on the existence pointer auth options.